Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

implement clean_chroot, #267

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

implement clean_chroot, #267

wants to merge 5 commits into from

Conversation

adrelanos
Copy link
Contributor

avoid host environment variables such as TMP to leak into the chroot

fixes #232

@adrelanos
implement clean_chroot,
221a31b 
avoid host environment variables such as TMP to leak into the chroot

fixes grml#232
@adrelanos
Copy link
Contributor Author

Some optional explanation why I made certain changes the way I made them.

If we use env -i then we can no longer export shell functions. So export -f "error_handler" had to be removed.

clean_chroot function has become more complex than we might have hoped for.

Even PATH needs to be set. Otherwise clean_chroot "$MNTPOINT" grub-install would fail because grub-install is in /usr/sbin/grub-install in the chroot.

A potential alternative that I personally would use is sudo as that already has sane environment scrubbing but I wasn't sure you would want a dependency on sudo. Also maybe in the future you would want to run most of grml-deboostrap as a unprivileged user and only use a privilege escalation tool (such as sudo) when needed and/or run commands unprivileged inside the chroot.

http_proxy has to be passed otherwise apt-cacher-ng would be broken by this commit. While at it, I completed it and added https_proxy, and ALL_PROXY there too for completeness sake.

Which environment variables are passed into the chroot is currently hardcoded. I didn't think it is important to be able to configure that. If somebody needs to configure that, I guess they can create a feature request and/or send a pull request.

The xtrace (bash -x) could be minimized if the code to set the environment variables was just run and by using global variables. That however would come at the cost at slightly more code complexity.

I was also wondering if it was better to use a similar mechanism to the one you're using for CHROOT_VARIABLES, but that would not work because only the chroot-script reads those. But you're not only using that but also other calls from grml-debootstrap to chroot (now clean_chroot) so the environment variables need to be set at the grml-debootstrap level.

@adrelanos
Copy link
Contributor Author

adrelanos commented Feb 3, 2024 •
edited
Loading

Resolved merge conflict.

@adrelanos
Copy link
Contributor Author

Ping?

mika
mika reviewed Apr 25, 2024
View reviewed changes
grml-debootstrap Outdated Show resolved Hide resolved
@adrelanos
Copy link
Contributor Author

Good now?

@adrelanos
Copy link
Contributor Author

Ping?

mika
mika reviewed Jun 7, 2024
View reviewed changes
grml-debootstrap Outdated Show resolved Hide resolved
mika
mika reviewed Jun 7, 2024
View reviewed changes
grml-debootstrap Outdated
einfo "End of debootstrap.log"
fi
## Check if "bailout" function is available.
## This is not the case in chroot-script.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I don't understand why this change ends up in this clean_chroot feature/commit, is the github web UI showing something unexpected? o_

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure where you saw that. But GitHub says Outdated". I cannot see any change related to / near ## Check if "bailout" function is available. in a local git diff.

mika
mika reviewed Jun 7, 2024
View reviewed changes
grml-debootstrap
@@ -26,7 +25,6 @@ set -e
set -E
set -o pipefail
trap "error_handler" ERR
export -f "error_handler"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is removing the export of the error_handler from grml-debootstrap and addition of it to chroot script related to the environment cleanup, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you mentioned this in a comment as I just noticed:

If we use env -i then we can no longer export shell functions. So export -f "error_handler" had to be removed.

We should squash all the relevant commits into one single commit and provide that useful information you collected into the commit message :)

Do you want me to give this a try or would you prefer to try on your own?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to give this a try

Yes, please.

@adrelanos
Copy link
Contributor Author

Ping?

@mika mika mentioned this pull request Aug 14, 2024
Open
@mika
Copy link
Member

mika commented Aug 14, 2024

Ping?

Sorry4delay, finally found some time to look into this, see #280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mika mika mika left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

libpam-tmpdir breaks grml-debootstrap
2 participants
@adrelanos @mika